Skip to content

Feature/hisq fgi#1368

Open
cpviolator wants to merge 5 commits into
developfrom
feature/hisq_fgi
Open

Feature/hisq fgi#1368
cpviolator wants to merge 5 commits into
developfrom
feature/hisq_fgi

Conversation

@cpviolator
Copy link
Copy Markdown
Member

This WIP add functionality to allow for offload in the FGI RHMC routines in https://github.com/callat-qcd/milc_qcd/tree/feature/nested-force-gradient-integrator. It simply creates copy gauge and momentum fields consistent with MILC's QUDA offload sequences.

@cpviolator cpviolator requested a review from a team as a code owner March 17, 2023 19:42
Comment thread lib/check_params.h
P(use_resident_mom, 0);
P(make_resident_gauge, 0);
P(make_resident_mom, 0);
P(use_fgi, 1);// FGI: This needs to be 0, and set to 1 with MILC/QUDA interface
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be set back to 0, I'm guessing?

Comment thread lib/interface_quda.cpp
cudaGaugeField *momResident = nullptr;
cudaGaugeField *extendedGaugeResident = nullptr;

// These Copy fields are to support the Force Gradient Integrator
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if I like the term "Copy"... maybe "Backup" would be a better term? "Previous"? I guess that would be best informed by the language of the FGI algorithm --- and if "copy" is the common language, that's good enough to me. (This also isn't a hill I'll die on!)

Comment thread lib/interface_quda.cpp
QudaReconstructType recon[4] = {gaugeSloppy->Reconstruct(), gaugePrecondition->Reconstruct(),
gaugeRefinement->Reconstruct(), gaugeEigensolver->Reconstruct()};

printfQuda("QudaInvertParam has different precisions than loadGaugeQuda, so recomputing sloppy fields\n");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the addition of this for logging reasons---I'd put this in a logQuda(QUDA_VERBOSE, [...]);, I think.

Comment thread lib/interface_quda.cpp
// recon is always no for fat links, so just use long reconstructs here
QudaReconstructType recon[4] = {gaugeLongSloppy->Reconstruct(), gaugeLongPrecondition->Reconstruct(),
gaugeLongRefinement->Reconstruct(), gaugeLongEigensolver->Reconstruct()};
printfQuda("QudaInvertParam has different precisions than loadGaugeQuda, so recomputing sloppy fields\n");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above

Comment thread lib/interface_quda.cpp

// Wilson Links
//----------------------------------------------------------
if(gaugePrecise) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just confirming --- it's intentional that this doesn't throw an errorQuda if gaugePrecise is unset, right? I see that it's fully safe code on its own.

Comment thread lib/interface_quda.cpp

// Wilson Links
//----------------------------------------------------------
if(gaugePrecise) gaugePrecise->copy(*gaugeCopyPrecise);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (gaugePrecise && gaugeCopyPrecise)

Comment thread lib/interface_quda.cpp

if(momResident) delete momResident;

// Delete MILC FGI fields
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know we discussed moving these into freeUniqueGauge, etc, offline --- just adding this comment so it can be marked as "resolved" at some point.

Comment thread include/quda.h
int return_result_gauge; /**< Return the result gauge field */
int return_result_mom; /**< Return the result momentum field */

int use_fgi; /**< add FGI terms */
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps this should be a bool instead? (Note that I'm going to kill QudaBoolean at some point, and replace it with bool - this is fine since bool is supported on C99 which we require anyway for the QUDA C interface.)

Comment thread include/quda.h
*/
void momResidentQuda(void *mom, QudaGaugeParam *param);

/**
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some punctuation in this comment might be nice :P

@maddyscientist
Copy link
Copy Markdown
Member

maddyscientist commented Mar 29, 2023

Nothing controversial in this commit. One thought on the design used here: would it be a better fit to push and pop gauge / momentum fields. E.g., instead of backing up the resident fields and zeroing them, and the later restoring them, we just push a new field (which pushes the resident field onto the proverbial stack) and when we're done with this temporary, we simply pop it, freeing the temporary and restoring the prior resident field off of the stack.

Just a thought.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants